Download refactor separate callback #1545
Conversation
VilppeRiskidev
force-pushed
the
download-refactor-separate-callback
branch
from
42bb355
to
0438561
Compare
| this.downloadingMod = false; | ||
| } | ||
| }); | ||
| await ModDownloadUtils.downloadCompletedCallback(this.profile, downloadedMods, this.$store); |
There was a problem hiding this comment.
It's currently possible for this to throw an unhandled exception and the next line never gets executed. (Granted, it was possible earlier too.)
| import ConflictManagementProvider from "../providers/generic/installing/ConflictManagementProvider"; | ||
| import { installModsToProfile } from "../utils/ProfileUtils"; | ||
|
|
||
| export async function downloadCompletedCallback(profile: Profile, downloadedMods: ThunderstoreCombo[], store: Store<any>) { |
There was a problem hiding this comment.
- I think calling this
downloadCompletedCallbackis not ideal, as it semantically binds it to the call site. Ideally a utility function would be a standalone thing. E.g. after your refactorings this might not be called as a callback anymore. If you end up doing it this way, I'd think about what this function actually does and rename it to describe that. - Also this function also deals with stuff that happens after the download is completed, so "download utils" is not the location I would place it into
- Showing the error modal, and thus the try-catch is IMO the UI components responsibility. One of the points of these refactorings is preparing for TCLI to implement these methods. It can't trigger a UI stuff in this manner, it has to be handled by the component.
- Same thing sort of applies to dispatching
'profile/updateModList', but to a lesser extend, as we can assume that's something the TCLI could implement internally. Generally speaking avoiding passing the Vuex store to an util function is probably a good idea, unless there's no way to work around it. - We want to get rid of
Profile, so a new function like this should useImmutableProfileas argument type. - Due to the above, I'm not sure this method is a very good refactoring candidate. It currently already calls different utility functions so there's not that much nitty-gritty business logic in the component. However, this is a bit of grey area, so if you want to proceed with this approach, take the comments above into consideration.
| import ProfileModList from '../../r2mm/mods/ProfileModList'; | ||
| import Profile from '../../model/Profile'; | ||
| import * as ModDownloadUtils from "../../utils/ModDownloadUtils"; |
There was a problem hiding this comment.
Nitpick: all the other imports here use single quotes.
| }, 1); | ||
| } | ||
|
|
||
| async downloadProgressCallback( |
There was a problem hiding this comment.
I had a long comment written here and now it's gone probably due to the review being in progress while the branch was force pushed. Short version: this change is good. But there are things to keep in mind for the next steps. Remember to ask me about "The Big Picture" in tomorrow's daily, as I refined the future plans a bit.
| @@ -23,3 +32,58 @@ export async function downloadCompletedCallback(profile: Profile, downloadedMods | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| export async function downloadProgressCallback ( | |||
There was a problem hiding this comment.
Having this as a separate method in the vue component makes sense, as it removed duplicated code. However, I'm not so sure having it as a utility function is a good idea. The fact that this takes 11 arguments which mostly deal with managing the component's internal state means that it's not really a standalone in anyway. It's so tightly coupled with the current component, I'd argue that having it in a separate file actually makes the code harder to read. Also The Big Picture stuff argues against moving this here.
No description provided.